Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CloudSyncMgr] Renaming the CloudSyncMgr API according to functionality to add more APIs #483

Merged
merged 6 commits into from
Feb 23, 2022

Conversation

nitu-s-gupta
Copy link
Contributor

@nitu-s-gupta nitu-s-gupta commented Feb 17, 2022

Description

The APIs were names as per the naming convection followed for previous APIs as for example, APIV1Request<><<GET/POST>> But with multiple APIs being added going further in CloudSyncMgr it is necessary to modify the API in terms of functionality i.e publish, subscribe

Fixes # (#382 )

Type of change

Please delete options that are not relevant.

  • Code cleanup/refactoring

How Has This Been Tested?

1.MQTT mosquitto broker is configured to be running in the AWS endpoint.
2. The edge orchestration is build and run using following command with option CLOUD_SYNC set to true

docker run -it -d --privileged --network="host" --name edge-orchestration -e CLOUD_SYNC=true -v /var/edge-orchestration/:/var/edge-orchestration/:rw -v /var/run/docker.sock:/var/run/docker.sock:rw -v /proc/:/process/:ro  lfedge/edge-home-orchestration-go:latest
  1. From the another terminal/post make a curl command as follows to publish data using home edge to the broker running on AWS endpoint
curl --location --request POST 'http://192.168.1.107:56001/api/v1/orchestration/cloudsyncmgr/publish' \
--header 'Content-Type: text/plain' \
--data-raw '{
    "appid": "testsamsung",
    "payload": "Testdata for subscribing",
    "topic": "homeedge/livingroom",
    "url" : "ec2-184-73-83-74.compute-1.amazonaws.com"
}'

4.You should obtain the following message for successful publish

{"Message":"Data published successfully to Cloud","RemoteTargetInfo":"","ServiceName":""}
  1. The Broker logs are checked in AWS and we observe following logs
1641480800: New connection from 49.207.219.60 on port 1883.
1641480800: New client connected from 49.207.219.60 as com.nokia.homeedge1 (p2, c1, k30).
1641480800: No will message specified.
1641480800: Sending CONNACK to com.nokia.homeedge1 (0, 0)
1641480800: Received PUBLISH from com.nokia.homeedge1 (d0, q0, r1, m0, 'home1/livingroom', ... (80 bytes))
1641480830: Received PINGREQ from com.nokia.homeedge1
1641480830: Sending PINGRESP to com.nokia.homeedge1
1641480850: Received PUBLISH from com.nokia.homeedge1 (d0, q0, r1, m0, 'home1/livingroom', ... (73 bytes))

Same client sending data but connection is established only once.

Test Configuration:

  • OS type & version: ( Ubuntu 20.04)
  • Hardware: (x86-64)
  • Edge Orchestration Release: (v1.1.0)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@nitu-s-gupta nitu-s-gupta changed the title Renaming the CloudSyncMgr API according to functionality to add more APIs [CloudSyncMgr] Renaming the CloudSyncMgr API according to functionality to add more APIs Feb 17, 2022
@@ -36,7 +36,7 @@ const (
type CloudSync interface {
InitiateCloudSync(isCloudSet string) error
//implemented by external REST API
RequestCloudSyncConf(host string, clientID string, message mqttmgr.Message, topic string) string
RequestCloudSyncPublish(host string, clientID string, message mqttmgr.Message, topic string) string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply, what about Publish?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean just Publish(), I thought of this idea, but then thought would be confusing for nay neww developer to understand by just Publish and hence this name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However this function is in the CloudSync. What about RequestPublish instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!!

@tiokim
Copy link
Contributor

tiokim commented Feb 18, 2022

Please correct How Has This Been Tested?.
I think you should fix the document as well.

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Feb 18, 2022
@nitu-s-gupta
Copy link
Contributor Author

Please correct How Has This Been Tested?. I think you should fix the document as well.

Done!!!

Copy link
Contributor

@suresh-lc suresh-lc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@nitu-s-gupta
Copy link
Contributor Author

@t25kim @MoonkiHong @tdrozdovsky PTAL 🙏

Copy link
Contributor

@tdrozdovsky tdrozdovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change

internal/orchestrationapi/mocks/mock_orchestration.go Outdated Show resolved Hide resolved
@tdrozdovsky
Copy link
Contributor

tdrozdovsky commented Feb 21, 2022

Please change:
docs/secure_manager.md:
Line 225: | /api/v1/orchestration/cloudsyncmgr | Allow | Allow | => | /api/v1/orchestration/cloudsyncmgr/publish | Allow | Allow |
Line 248: p, member, /api/v1/orchestration/cloudsyncmgr, * => p, member, /api/v1/orchestration/cloudsyncmgr/publish, *
internal/controller/securemgr/authorizer/authorizer.go:
Line 39: "p, member, /api/v1/orchestration/cloudsyncmgr, *\n" => "p, member, /api/v1/orchestration/cloudsyncmgr/publish, *\n
internal/restinterface/externalhandler/externalhandler_test.go
Line:292: func TestAPIV1RequestCloudSyncmgrPost(t *testing.T) { => func TestAPIV1RequestCloudSyncmgrPublish(t *testing.T) {

@boring-cyborg boring-cyborg bot added the security Any tasks and issues w.r.t. the security label Feb 21, 2022
@nitu-s-gupta
Copy link
Contributor Author

Please change: docs/secure_manager.md: Line 225: | /api/v1/orchestration/cloudsyncmgr | Allow | Allow | => | /api/v1/orchestration/cloudsyncmgr/publish | Allow | Allow | Line 248: p, member, /api/v1/orchestration/cloudsyncmgr, * => p, member, /api/v1/orchestration/cloudsyncmgr/publish, * internal/controller/securemgr/authorizer/authorizer.go: Line 39: "p, member, /api/v1/orchestration/cloudsyncmgr, *\n" => "p, member, /api/v1/orchestration/cloudsyncmgr/publish, *\n internal/restinterface/externalhandler/externalhandler_test.go Line:292: func TestAPIV1RequestCloudSyncmgrPost(t *testing.T) { => func TestAPIV1RequestCloudSyncmgrPublish(t *testing.T) {

Done!!

@sonarcloud
Copy link

sonarcloud bot commented Feb 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.9% 2.9% Duplication

@tdrozdovsky tdrozdovsky self-requested a review February 21, 2022 14:27
Copy link
Contributor

@tdrozdovsky tdrozdovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@tiokim tiokim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks!

Copy link
Contributor

@MoonkiHong MoonkiHong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@MoonkiHong MoonkiHong merged commit dfd3e6e into lf-edge:master Feb 23, 2022
@nitu-s-gupta nitu-s-gupta deleted the publish branch February 24, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base code documentation Improvements or additions to documentation security Any tasks and issues w.r.t. the security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants